Do not allocate more resources than the maxTotal
#394
Conversation
There was a problem hiding this comment.
This looks right to me.
I wonder whether it obsoletes this check? The old behavior was that the setting enforced a max idle. But if we can't go above the max at any point, we can't have more than the max idling. The check could be useful for a renamed property, maxIdle, which I think would be in harmony with how commons-pool2 works.
|
Unfortunately, I do not see a simple way of testing |
|
Currently, I have a bit of spare time and I can work on some improvements:
I assume this work can be scheduled for 0.5.x release? |
|
I would like to eventually integrate this with otel4s. It would be interesting to have an introspectable backend for metrics. |
|
I was keeping an eye on otel4s for a while. I can experiment with it within keypool. |
|
That would be fantastic! Regarding 0.4 vs. 0.5, we're in that weird "early semver" territory.
Wherever this change lands, we need to shout about it in the release notes and in the http4s release notes, but I also think it's important enough it needs to be out there. |
|
I'm still not sure whether this is an 0.4 or 0.5. I'm inclined toward it being a bugfix and continuing to call this 0.4. |
| @@ -402,6 +416,7 @@ object KeyPool { | |||
| val defaultReuseState = Reusable.Reuse | |||
| val idleTimeAllowedInPool = 30.seconds | |||
| def maxPerKey[K](k: K): Int = Function.const(100)(k) | |||
| val maxIdle = 100 | |||
There was a problem hiding this comment.
Can we have maxIdle equal to maxTotal always?
There was a problem hiding this comment.
Good point!
Basically, the rule should be this one: maxIdle <= maxTotal
So the builder method can be defined as the following:
def withMaxTotal(total: Int) =
copy(maxTotal = total, maxIdle = math.min(maxIdle, total))Should withMaxIdle throw an exception when maxIdle > maxTotal?
There was a problem hiding this comment.
commons-pool2 has three independent settings: maxTotal, maxIdle, and minIdle.
- maxTotal is how many you can handle in a burst
- maxIdle and minIdle let you tune how many you can afford to keep warm in order to usually have one ready under normal load
minIdle <= maxIdle <= maxTotal ought to always be true. minIdle == maxIdle == maxTotal will tend to be true when creating them is expensive and idling them is cheap.
There was a problem hiding this comment.
In an ideal world, I would expect a builder to have validation logic under the hood.
Neither negative maxTotal nor maxIdle makes sense.
There was a problem hiding this comment.
It would be surprising to me if withMaxTotal implemented the math.min. Imagine reading from a config, and withMaxTotal got called before withMaxIdle.
I can imagine implementing math.min at creation time, logging a warning (http4s-blaze does this with timeout values), or raising an error.
# Conflicts: # core/src/main/scala/org/typelevel/keypool/KeyPool.scala # core/src/main/scala/org/typelevel/keypool/KeyPoolBuilder.scala
|
This is major forward progress and shouldn't be blocked by validations. Configuring maxIdle > maxTotal is wrong, but harmless: we'll just never hit that many. We can add a warning, or validate non-negative, but that can be a separate PR. |
My attempt to fix #389.
I assume,
maxPerKeyshould behave in the same way?